Add Thrift-native metadata support - #1641
Conversation
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — solid, well-tested opt-in feature with strong unit coverage across the native/legacy split, error-code/SQLState alignment, and JSON round-trip. One low-severity note about asymmetric native error propagation (listTables/listColumns propagate errors under native mode, but listFunctions/key-based ops still swallow object-not-found) — worth confirming it's intentional.
d535038 to
76205fa
Compare
There was a problem hiding this comment.
Verdict: 1 Medium · 1 Low
Solid, well-tested opt-in feature; native metadata routing delegates cleanly to existing row builders and the SQLState/errorCode alignment looks correct. One medium concern: the fallback-bypass guard keys off the connection flag rather than the actual result, so enabling EnableThriftNativeMetadata changes getColumns/getTables error semantics even for legacy (non-native) responses. One low note on a tautological test.
76205fa to
0a99a83
Compare
There was a problem hiding this comment.
Verdict: 1 Medium · 2 Low
Solid, well-tested feature addition — opt-in Thrift-native SEA metadata with the manifest flag as the authoritative switch, good coverage across builders and header wiring. Main concerns are around the enabled-flag error-handling path: enabling the flag disables old-DBR SHOW fallbacks for the whole connection (F1), an inconsistent SQLState between null vs empty table (F2), and the native-propagation guard not applied uniformly to key-based ops (F3). All are behavior/consistency questions rather than clear defects.
0750722 to
6eac609
Compare
There was a problem hiding this comment.
Verdict: 1 Low
Solid, well-structured feature with strong test coverage (manifest-flag gating, native vs SHOW paths, SQLState/error-code corrections, and the new post-filtering are all exercised). One low-severity robustness observation about the implicit positional-column contract between native results and the builders; otherwise the correctness, error-propagation, and JSON round-trip changes look correct.
6eac609 to
e22b219
Compare
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
e22b219 to
9273799
Compare
There was a problem hiding this comment.
Verdict: 1 Medium
Solid, well-tested opt-in feature. The manifest-flag-authoritative design is clean and the new unit coverage (native-vs-thrift equivalence, filter preservation, SQLState corrections, header behavior) is thorough. One medium note about inconsistent error-fallback behavior between metadata operations when the flag is enabled.
There was a problem hiding this comment.
Verdict: 1 Medium · 1 Low
Solid, well-tested feature addition — the manifest-authoritative native-metadata path, SQLState corrections, and Thrift error-code preservation all carry matching unit coverage. One medium concern: the native error-propagation guard keys off the feature flag rather than the actual result, which can disable the legacy object-not-found/parse-error fallbacks (and make getTables/getColumns throw instead of returning empty) even for non-native SEA results; plus a low note on that policy being applied inconsistently vs. getFunctions.
| getResultSet(SQL, session, MetadataOperationType.GET_TABLES), validatedTableTypes); | ||
| resultSet, requestedCatalog, resultTableTypes); | ||
| } catch (SQLException e) { | ||
| if (isThriftNativeMetadataRequested()) { |
There was a problem hiding this comment.
🟡 Medium — The error-propagation guard keys off isThriftNativeMetadataRequested() (i.e. whether the feature flag is enabled) rather than whether the result was actually native. Because the exception is thrown by getResultSet(...) before any manifest is available, there is no way at this point to know if the server actually returned a native result. The PR design states the manifest flag is authoritative and legacy behavior is preserved when the server does not return a native result — but here, once EnableThriftNativeMetadata=1, both listTables (L176) and listColumns (L227) short-circuit all the legacy fallbacks, including isObjectNotFoundException(e).
Concrete consequence: with the flag enabled against a server that does not support native metadata (older DBR that ignores the header and runs the SHOW query, or a genuinely non-existent catalog/object), getTables/getColumns now throw instead of returning an empty ResultSet. Returning empty for object-not-found is the pre-existing JDBC-contract behavior for these calls. Enabling this opt-in flag therefore silently regresses the object-not-found and SHOW ... IN ALL CATALOGS parse-error fallbacks even when the server produced a legacy SEA result. Worth confirming this is the intended tradeoff (and documenting it), or gating the propagation on something closer to "native result was actually returned."
| getResultSet(SQL, session, MetadataOperationType.GET_FUNCTIONS); | ||
| return metadataResultSetBuilder.getFunctionsResult( | ||
| getResultSet(SQL, session, MetadataOperationType.GET_FUNCTIONS), catalog); | ||
| resultSet, resultSet.isThriftNativeMetadataResult() ? requestedCatalog : catalog); |
There was a problem hiding this comment.
🔵 Low — The native error-handling policy is applied inconsistently across the supported operations. listTables (L176) and listColumns (L227) rethrow immediately when isThriftNativeMetadataRequested(), bypassing their isObjectNotFoundException fallback, but listFunctions (here) keeps its isObjectNotFoundException(e) → empty-result fallback with no equivalent native guard. If the intent is Thrift-compatible propagation for native metadata, functions diverges from tables/columns; if returning empty on object-not-found is the desired native behavior, then tables/columns diverge instead. Please confirm the intended policy is uniform, or add a comment explaining why functions is treated differently.
(Anchored to the nearest changed line — see the description for the exact location.)
sreekanth-db
left a comment
sreekanth-db
left a comment
There was a problem hiding this comment.
getTables() and getColumns() use SQL fallbacks for unsupported SHOW ... IN ALL CATALOGS syntax and return empty results for object-not-found or empty-pattern cases.
Can we test the new flow for these cases against older-version compute that does not support the native metadata header?
There was a problem hiding this comment.
Verdict: 1 Low
Looks solid overall — the opt-in EnableThriftNativeMetadata path is well-tested (manifest-flag authority, native filtering for tables/functions/cross-refs, SQLState/error-code corrections) and the catalog.equals(row.get(0)) flip correctly hardens against null catalog cells. One low-confidence note: the native error-propagation guard added to listTables/listColumns is not mirrored in listImportedKeys/listCrossReferences/listFunctions/listPrimaryKeys, which still fall back to empty results — likely benign for object-not-found but inconsistent with the PR's stated propagation design. Note also that the error-code change (INVALID_STATE → EXECUTE_STATEMENT_FAILED) reuses an existing code and has test coverage, so no new taxonomy entry appears required.
| LOGGER.debug("listExportedKeys: table is null, throwing"); | ||
| throw new DatabricksSQLException( | ||
| "Invalid argument: tableName may not be null", DatabricksDriverErrorCode.INVALID_STATE); | ||
| "Invalid argument: tableName may not be null", |
There was a problem hiding this comment.
🔵 Low — Native error-propagation guard is applied inconsistently across metadata operations. listTables (L176) and listColumns (L227) added if (isThriftNativeMetadataRequested()) throw e; before their legacy SHOW-fallbacks, so native failures propagate as the PR describes. But listImportedKeys (this line) and listCrossReferences (L493) — as well as listFunctions and listPrimaryKeys — retain their catch blocks that swallow PARSE_SYNTAX_ERROR_SQL_STATE/object-not-found and return an empty result set, with no native guard.
For object-not-found this matches Thrift's empty-result behavior, so it is likely benign. But a genuine native-path failure surfacing as 42601 (or an object-not-found classification) on these operations will still be silently converted to an empty result set rather than propagated, which contradicts the stated design and diverges from the tables/columns paths. Worth confirming this asymmetry is intentional.
(Anchored to the nearest changed line — see the description for the exact location.)
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a clean, well-tested opt-in feature; only 1 low-severity dead-code note. Verified the native-path row copies are mutable (so downstream row.set(...) is safe), that immutable List.of(...) from listCatalogs flows only into the copy-based getThriftRows, and that the added null checks / catalog.equals(row.get(0)) flip are genuine improvements. Nit (summary-only): the key-based validation deliberately maps null-table → 42000 but empty-table/other → 08000; the asymmetry is surprising but is intentional Thrift-compat behavior and is covered by tests, so no change requested.
| && !queryExecutionClient.getConnectionContext().getEnableMultipleCatalogSupport(); | ||
| } | ||
|
|
||
| private boolean isThriftNativeMetadataRequested() { |
There was a problem hiding this comment.
🔵 Low — isThriftNativeMetadataRequested() is introduced but never called anywhere in the module. The native-vs-SEA decision is actually driven off resultSet.isThriftNativeMetadataResult() (manifest flag) inside listTables/listFunctions, and the request-header opt-in is decided in DatabricksSdkClient.executeStatement. This helper duplicates the connection-context check that already lives in DatabricksSdkClient and is dead code. Consider removing it to avoid the impression that the query client gates native requests (it does not — the manifest flag is authoritative).
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good overall — a well-structured, opt-in feature with strong unit coverage (manifest-flag authority, native post-filtering for tables/functions/cross-refs, and SQLState/errorCode assertions). Verified the DatabricksSQLException constructor overload used in DatabricksThriftAccessor (now preserves the source errorCode — an improvement), the getTablesResult signature migration across all callers, and null-safety of the new matchesParent/copyThriftNativeMetadataRows paths; no correctness issues found. One low-severity note on an inconsistent SQLState class for null- vs empty-table validation. Separately, per CLAUDE.md's telemetry-error guidance, the reclassification of key-based validation errors from INVALID_STATE to EXECUTE_STATEMENT_FAILED should have the driver/server/user classification recorded in the maintainers' telemetry taxonomy — worth confirming that was linked in the PR.
| throw new DatabricksSQLException( | ||
| "Invalid argument: tableName may not be null or empty", | ||
| DatabricksDriverErrorCode.INVALID_STATE); | ||
| OPERATION_ERROR_SQLSTATE, |
There was a problem hiding this comment.
🔵 Low — For the same validation message ("Invalid argument: tableName may not be null or empty"), a null table throws with SQLState 42000 (syntax/access rule violation) while an empty table throws with SQLState 08000 (OPERATION_ERROR_SQLSTATE). SQLState class 08 denotes connection exception, which is semantically wrong for an argument-validation failure — a caller branching on the SQLState class would treat an empty-string table name as a connection problem. This also diverges from listExportedKeys (line 419), where a null table correctly uses 42000. The two null/empty branches for identical error text emitting different SQLState classes reads as an oversight rather than intent.
If this deliberately mirrors what the Thrift server returns for empty identifiers, a brief code comment would prevent it from looking like a copy/paste divergence. Otherwise consider using 42000 for both.
2 participants
Summary
Adds opt-in
EnableThriftNativeMetadatasupport for metadata operations executed through SEA. The server can return Thrift-shaped metadata rows through the Statement Execution API, while the driver preserves the same filtering, normalization, error behavior, and JDBC metadata exposed by the Thrift client.Supported operations are catalogs, schemas, tables, columns, functions, primary keys, and cross references. Procedures and procedure columns continue to use the existing SEA path.
Request and result flow
The manifest flag is authoritative: neither the request header nor a native-looking schema changes how a result is processed. This keeps the feature opt-in and preserves legacy behavior when the server does not return a native result.
Requests requiring special handling
getTables: the native path sends catalog and types to runtime, then reapplies the JDBC filters to returned rows. This is necessary because runtime treats catalog as a pattern, may return temporary views outside the requested catalog, and does not consistently handle empty or exact table-type filters. SEA native results reuse this processing to preserve existing JDBC-over-Thrift behavior.typesbehaviornullaccepts every runtime-returned type, an empty array returns no rows, and a non-empty array is matched exactlynulluses the driver's supported default types because SHOW has no JDBCtypesargumentgetFunctions: the runtime paths have different catalog semantics:FUNCTION_CAT""DatabaseMetaData.getFunctionscatalog, includingnullThe driver therefore saves the original JDBC catalog before resolving a catalog for SQL construction and passes that original value only when rebuilding a manifest-confirmed native result. This preserves Thrift compatibility, but it is a column-label correction rather than catalog filtering: native rows can come from catalog A and be labeled as catalog B.
getCrossReference: the SQL query narrows only the foreign-key side, so the returned native rows are additionally filtered by the requested parent catalog, schema, and table.Native metadata failures use Thrift-compatible propagation and timeout codes instead of the legacy SHOW-query compatibility fallbacks.
SQLState correction
Key-based metadata validation now reports the applicable SQLState (
42000or08000) and keepsEXECUTE_STATEMENT_FAILED(1003) as the driver error code. Previously these errors were constructed withDatabricksDriverErrorCode.INVALID_STATE, which incorrectly exposedINVALID_STATEthroughSQLException.getSQLState().Tests:
mvn spotless:check; focused core metadata suites (331 tests).